-
Notifications
You must be signed in to change notification settings - Fork 76
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent PR! I left some questions and comments :)
Co-authored-by: Andrew Fleming <[email protected]>
… into account-declare-deploy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small fix on the click option, but otherwise this looks good to me!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
account's functions should set max_fee
to be None
by default (the functions' code already handles None
but the function signatures need to reflect this)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a couple of comments, but I can provide a better review after conflicts with main are resolved, because there are important differences regarding the int pattern mostly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, @martriay! I left a few comments :)
The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! I added a comment and thumbs up to the Andrew review notes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good to go!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go!
c65d8ac
Fixes #186, #205.
run_command
so it actually centralizes starknet CLI calls.nile.core.declare.get_hash
which takes acontract_class
and returns its hash (see Improve howdeclare
handles an existing class hash #205).Account.deploy_contract
which relies on a Deployer Contract to deploy, which eludes our deployments tracking/management system. I think we need to rethink its model, but probably in a different issue/PR.